Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InstructionDurations from BackendV2 #11528

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

MozammilQ
Copy link
Contributor

Summary

This PR adds the support for BackendV2 in method from_backend of class InstructionDurations
in qiskit.transpiler.instruction_durations.py

Credit:
@nkanazawa1989 , for help and ideas :)

Details and comments

Earlier, InstructionDurations.from_backend() could support only BackendV1.
In this PR, now BackendV2 can also be passed into method from_backend.

@MozammilQ MozammilQ requested a review from a team as a code owner January 9, 2024 17:25
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 9, 2024
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

Comment on lines 106 to 126
elif isinstance(backend, BackendV2):
target = backend.target
inst_with_no_props = {"delay"}
inst_duration = None
for name in target.operation_names:
for qubits, inst_props in target._gate_map[name].items():
if name in inst_with_no_props:
# Setting the duration for 'delay' to zero.
inst_duration = 0.0
else:
try:
inst_duration = inst_props.duration
except AttributeError:
logger.info("%s on %s did not report any duration", name, qubits)
continue
instruction_durations.append((name, qubits, inst_duration, "s"))

try:
dt = target.dt
except AttributeError:
logger.info("Backend Target didn't report any dt")
Copy link
Member

@mtreinish mtreinish Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use backend.target.durations() (https://docs.quantum.ibm.com/api/qiskit/qiskit.transpiler.Target#durations)? It should already have most of this logic present in there so you can make this branch one line.

Copy link
Contributor Author

@MozammilQ MozammilQ Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtreinish , Haha! :) I was working on a different PR, I discovered InstructionDurations.from_backend() didn't have the BackendV2 and did this PR.
A much better way: backend.target.durations() didn't cross my mind :)
Thanks, for the suggestion :)
I will add the suggestion :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtreinish , added suggestion, please see if this is good enough :)

@coveralls
Copy link

coveralls commented Jan 9, 2024

Pull Request Test Coverage Report for Build 8315482005

Details

  • 22 of 22 (100.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.008%) to 89.296%

Files with Coverage Reduction New Missed Lines %
qiskit/compiler/transpiler.py 2 91.96%
crates/qasm2/src/lex.rs 2 92.19%
crates/qasm2/src/parse.rs 6 97.15%
Totals Coverage Status
Change from base Build 8310563987: 0.008%
Covered Lines: 59791
Relevant Lines: 66958

💛 - Coveralls

Comment on lines 20 to 21
Given :code:`dt` and :code:`dtm` for a :class:`.BackendV1` or :class:`.BackendV2` are unequal,
:meth:`~.InstructionDurations.from_backend` does not raise any error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any change in code behavior with this PR? dtm is acquisition sampling rate and doesn't impact the instruction duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nkanazawa1989 ,
According to,

TranspilerError: If dt and dtm is different in the backend.

I should get TranspilerError if dt!=dtm, I just checked it, and found it doesn't.
So, I added this line in release note and added test test_works_unequal_dt_dtm to make sure this case is covered :)

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The document should be wrong. See p.33 of https://arxiv.org/abs/1809.03452. dtm defines the sampling rate of digitizer, which can be independent of dt that defines the sampling rate of AWG.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this upgrade note is not necessary because TranspilerError was not actually raised (just accidentally documented).

with self.assertRaises(TranspilerError):
durations.get(self.example_gate, self.example_qubit[0])

def test_works_unequal_dt_dtm(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this test? InstructionDuration is the object used for scheduling, and dtm doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nkanazawa1989 , This line was the motivation:

TranspilerError: If dt and dtm is different in the backend.

Am I missing something

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MozammilQ for updating the test and error handling. Several minor fix would be required but overall this PR is good to go.

Comment on lines 20 to 21
Given :code:`dt` and :code:`dtm` for a :class:`.BackendV1` or :class:`.BackendV2` are unequal,
:meth:`~.InstructionDurations.from_backend` does not raise any error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this upgrade note is not necessary because TranspilerError was not actually raised (just accidentally documented).

test/python/transpiler/test_instruction_durations.py Outdated Show resolved Hide resolved
test/python/transpiler/test_instruction_durations.py Outdated Show resolved Hide resolved
test/python/transpiler/test_instruction_durations.py Outdated Show resolved Hide resolved
@nkanazawa1989 nkanazawa1989 added the mod: transpiler Issues and PRs related to Transpiler label Feb 15, 2024
@nkanazawa1989 nkanazawa1989 self-assigned this Feb 15, 2024
@MozammilQ
Copy link
Contributor Author

MozammilQ commented Feb 15, 2024

@nkanazawa1989 , see if this is good enough :)

@jakelishman
Copy link
Member

@dieris: this PR is generally needed on Qiskit (for Qiskit 1.1 in 3 months' time), but I suspect might have some effects for work you did recently on qiskit-ibm-provider (and/or runtime) in Qiskit/qiskit-ibm-provider#787. Just flicking very quickly through the ibm-provider code, it looks like it'll handle things fine, but I'm just flagging this to you in case it affects anything.

@jakelishman jakelishman added the Changelog: New Feature Include in the "Added" section of the changelog label Feb 15, 2024
@jakelishman jakelishman added this to the 1.1.0 milestone Feb 15, 2024
@dieris
Copy link
Contributor

dieris commented Feb 15, 2024

@dieris: this PR is generally needed on Qiskit (for Qiskit 1.1 in 3 months' time), but I suspect might have some effects for work you did recently on qiskit-ibm-provider (and/or runtime) in Qiskit/qiskit-ibm-provider#787. Just flicking very quickly through the ibm-provider code, it looks like it'll handle things fine, but I'm just flagging this to you in case it affects anything.

yes, it looks like we had a similar idea, but the from_backend method in runtime/provider just overrides this one for the DynamicCircuitInstructionDurations subclass, so there are no conflicts.

Support for :class:`.BackendV2` in :meth:`.InstructionDurations.from_backend` is added.

Users can have an object of :class:`.InstructionDurations` using :class:`.BackendV2`
from :meth:`.from_backend` with followig code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from :meth:`.from_backend` with followig code.
from :meth:`.InstructionDurations.from_backend` with followig code.

to fix doc error

nkanazawa1989
nkanazawa1989 previously approved these changes Feb 20, 2024
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MozammilQ this looks good to me now, except for the docs generation failure. Can we merge this once all test pass?

@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , Please, see if this is good enough :)

@jakelishman jakelishman modified the milestones: 1.1.0, 1.2.0 Apr 11, 2024
@ElePT ElePT removed this from the 1.2.0 milestone Jul 23, 2024
@MozammilQ
Copy link
Contributor Author

@1ucian0 ,
pulse features are being deprecated in Qiskit, and being moved to Qiskit Dynamics, is this PR required anymore?
If yes, in what direction the PR should be heading?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: transpiler Issues and PRs related to Transpiler
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

8 participants